-
Notifications
You must be signed in to change notification settings - Fork 96
JS code can now catch Ruby exceptions #373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Before this commit exceptions from Ruby callbacks were translated to termination exceptions that cannot be caught by JS code. Turn them into regular exceptions that can be caught. Fixes: rubyjs#357
|
I think the truffelruby tests are not supposed to fail 🤔 |
|
I disabled the test for truffleruby. Ideally truffleruby and mruby should behave the same but I don't know enough about the former to say whether that's feasible. |
|
That "corrupted double-linked list" test failure on the I tried |
|
The error went away after a rerun. I ran valgrind locally a couple of times but I've not been able to definitely pinpoint any problems in mini_racer itself. As I mentioned earlier, there are plenty of warnings, and mini_racer.so shows up in some of the stack traces, but the top-most frame is always some internal ruby function operating on a VALUE. I've not been able to trace any of those back to mini_racer not initializing a VALUE properly. I can't conclusively say it's not a mini_racer issue (the data is too tenuous) but there are no obvious smoking guns. |
|
IIRC, @eregon took a look from time to time to mini_racer's truffleruby support, so I'm just pinging him for FYI. |
| context = MiniRacer::Context.new | ||
| context.attach("test", proc { raise "boom" }) | ||
| actual = context.eval("try { test() } catch (e) { e }") | ||
| assert_equal(actual.class, MiniRacer::ScriptError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tweaked it so the JS exception is deserialized to a MiniRacer::ScriptError Ruby exception now.
That should be implementable easily in truffleruby too, I think?
|
I'll try to take a look. |
|
I've pushed a commit to make the test pass on TruffleRuby: main...eregon:mini_racer:fix357-and-truffleruby |
…alled from JS back to Ruby for the TruffleRuby backend
|
Thanks @eregon, works great. Anyone wants to LGTM this? |
tisba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C related changes are quite a bit out of my area of expertise. The Ruby tests look good, no more failing tests and since you two worked on this, I'd say, let's go!
This is a really nice addition to make mini_racer's behavior less surprising. Thank you! 🙇
Before this commit exceptions from Ruby callbacks were translated to termination exceptions that cannot be caught by JS code. Turn them into regular exceptions that can be caught.
Fixes: #357